fuzz: add fuzz target for P2PGossipSync gossip message handling by NishantBansal2003 · Pull Request #4532 · lightningdevkit/rust-lightning

NishantBansal2003 · 2026-04-01T15:43:23Z

Added gossip state machine fuzz tests for gossip discovery state handling. In these fuzz tests, we read bytes from the fuzz input to determine actions such as connecting peers, feeding channel announcements, node announcements, channel updates, or query messages. Both valid and malformed messages are generated to exercise error paths.

The reason for adding these fuzz tests is that P2PGossipSync is the ultimate sink for gossip messages received from peers, so there must not be any crashes or unintended behavior in LDK when handling messages from potentially malicious peers.

I only included the states that LDK currently handles. For example, I did not add reply_channel_range or query_scids messages since LDK does not process them at the moment. Similarly, since P2PGossipSync does not handle gossip_timestamp_filter, it is not included in these fuzz tests. Additionally, there is limited benefit in fuzzing gossip_timestamp_filter in LDK, as it is typically processed only once per peer and involves minimal logical processing.

ldk-reviews-bot · 2026-04-01T15:43:26Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>

ldk-claude-review-bot · 2026-04-01T15:56:07Z

fuzz/src/gossip_discovery.rs

+
+						let graph = network_graph.read_only();
+						let node_id = NodeId::from_pubkey(&peer.node_id);
+						let node = graph.node(&node_id).unwrap();
+						let info = node.announcement_info.as_ref().unwrap();
+						assert_eq!(info.last_update(), timestamp);


The Ok branch assertions look up peer.node_id (the original, pre-mutation value), but if the node_id was mutated at line 184-186, the accepted announcement would be stored under ann.contents.node_id (the mutated value). In secp256k1_fuzz mode, signature verification is simplified and could theoretically allow a mutated node_id to pass, causing graph.node(&NodeId::from_pubkey(&peer.node_id)).unwrap() to panic on a node that was never updated.

To be robust regardless of fuzz-mode crypto behavior, this should use the actual message values:

Suggested change

let graph = network_graph.read_only();

let node_id = NodeId::from_pubkey(&peer.node_id);

let node = graph.node(&node_id).unwrap();

let info = node.announcement_info.as_ref().unwrap();

assert_eq!(info.last_update(), timestamp);

let graph = network_graph.read_only();

let node = graph.node(&ann.contents.node_id).unwrap();

let info = node.announcement_info.as_ref().unwrap();

assert_eq!(info.last_update(), timestamp);

ldk-claude-review-bot · 2026-04-01T15:56:16Z

fuzz/src/gossip_discovery.rs

+						let graph = network_graph.read_only();
+						let chan = graph.channel(scid).unwrap();
+						assert_eq!(chan.node_one, NodeId::from_pubkey(&peer1.node_id));
+						assert_eq!(chan.node_two, NodeId::from_pubkey(&peer2.node_id));
+
+						let node1_id = NodeId::from_pubkey(&peer1.node_id);
+						let node2_id = NodeId::from_pubkey(&peer2.node_id);
+						assert!(graph.node(&node1_id).is_some());
+						assert!(graph.node(&node2_id).is_some());


Same issue as the node announcement case: these assertions use the original peer1.node_id/peer2.node_id, but the node_id_1/node_id_2 fields may have been mutated at lines 238-246. If a mutation passes signature verification in secp256k1_fuzz mode, chan.node_one/chan.node_two would hold the mutated values, causing the assert_eq! to fail spuriously.

Use the actual announcement contents instead:

Suggested change

let graph = network_graph.read_only();

let chan = graph.channel(scid).unwrap();

assert_eq!(chan.node_one, NodeId::from_pubkey(&peer1.node_id));

assert_eq!(chan.node_two, NodeId::from_pubkey(&peer2.node_id));

let node1_id = NodeId::from_pubkey(&peer1.node_id);

let node2_id = NodeId::from_pubkey(&peer2.node_id);

assert!(graph.node(&node1_id).is_some());

assert!(graph.node(&node2_id).is_some());

let graph = network_graph.read_only();

let chan = graph.channel(scid).unwrap();

assert_eq!(chan.node_one, ann.contents.node_id_1);

assert_eq!(chan.node_two, ann.contents.node_id_2);

assert!(graph.node(&ann.contents.node_id_1).is_some());

assert!(graph.node(&ann.contents.node_id_2).is_some());

ldk-claude-review-bot · 2026-04-01T15:56:50Z

I've thoroughly reviewed every file and hunk in this diff. All three issues from the prior review have been addressed:

The licensing header is now present at the top of gossip_discovery.rs.
The node announcement Ok branch now correctly uses node_ann.contents.node_id instead of peer.node_id.
The channel announcement Ok branch now correctly uses chan_ann.contents.node_id_1/node_id_2 instead of peer1.node_id/peer2.node_id.

No new issues found. The fuzz target logic is sound: the FuzzUtxoLookup mock returns sync results correctly, the maybe_set! macro properly exercises both valid and invalid message paths, decode error handling is complete, event assertions for peer_connected and handle_query_channel_range are correct (verified against the P2PGossipSync implementation), and the NetworkUpdate assertions are valid for is_permanent: true. The generated target file matches the standard template. All imports are used.

ldk-claude-review-bot · 2026-04-01T16:02:32Z

fuzz/src/gossip_discovery.rs

@@ -0,0 +1,462 @@
+//! Test that no series of gossip messages received from peers can result in a crash. We do this


Nit: This file is missing the standard licensing header that other fuzz targets (e.g., full_stack.rs, chanmon_consistency.rs, feature_flags.rs) include at the top:

// This file is Copyright its original authors, visible in version control // history. // // This file is licensed under the Apache License, Version 2.0 <LICENSE-APACHE // or http://www.apache.org/licenses/LICENSE-2.0> or the MIT license // <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your option. // You may not use this file except in accordance with one or both of these // licenses.

The doc comment (//! Test that ...) should come after this header.

codecov · 2026-04-01T16:51:43Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.11%. Comparing base (450c03a) to head (d29150d).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4532   +/-   ##
=======================================
  Coverage   87.10%   87.11%           
=======================================
  Files         163      163           
  Lines      108740   108740           
  Branches   108740   108740           
=======================================
+ Hits        94723    94730    +7     
+ Misses      11531    11526    -5     
+ Partials     2486     2484    -2

Flag	Coverage Δ
fuzzing	`40.35% <ø> (+0.15%)`	⬆️
tests	`86.20% <ø> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

TheBlueMatt

Cool, thanks! Have you run any comparisons on the coverage of this vs the router fuzzer? I'm wondering if it makes sense to add a new fuzzer here or focus more CPU on the router fuzzer given it tests very similar codepaths.

NishantBansal2003 · 2026-04-02T15:33:35Z

Cool, thanks! Have you run any comparisons on the coverage of this vs the router fuzzer? I'm wondering if it makes sense to add a new fuzzer here or focus more CPU on the router fuzzer given it tests very similar codepaths.

Coverage wise, the additional benefit is that P2PGossipSync also gets covered, so that's a plus.

My main goal was to cover all announcement messages in the fuzz tests (which the router fuzzer already does), and additionally support pruning cases like fail channel (already covered), fail node, and stale channel, along with handling gossip queries. However, since LDK only supports query channel range, we effectively get only one additional path to fuzz.

One option is to extend the router.rs fuzzer by adding new states (query channel range, fail node, stale channel) and checking invariants at the end of each state also. But for this, we would also need to include P2P gossip sync there.

OR, we can keep a separate fuzz target (gossip_discovery.rs) dedicated purely to gossip related cases (which was my original intention). The downside is some duplication in CPU usage with router.rs.

WDYT? I'm fine with either approach since both will ultimately achieve full coverage of gossip message processing.

ldk-reviews-bot · 2026-04-04T00:00:45Z

🔔 1st Reminder

Hey @joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-06T00:01:46Z

🔔 2nd Reminder

Hey @joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

TheBlueMatt

Hmm, yea, I suppose the goal is sufficiently different that it makes sense to just ship a separate fuzzer. What could go wrong, anyway? :)

TheBlueMatt · 2026-04-06T12:02:22Z

fuzz/src/gossip_discovery.rs

+					excess_data,
+				};
+
+				if mutate_field!() {


I'm not entirely sure its worth all the effort for creative message building - should we instead just read a length and then deserialize N bytes as each message and process it?

I followed your suggestion and used a decode method to fill the inputs (mostly taken from router.rs). The reason for doing this is that the code is now somewhat reduced. But, the downside is that some fuzz input gets wasted due to being overwritten. I think it’s fine?

The reason I initially did manual field parsing is i want the fuzz tests to cover P2P gossip sync e2e, including sign verify and raw fuzz input won't penetrate sign verification, so we need to define some valid fields manually

including sign verify and raw fuzz input won't penetrate sign verification

We deliberately use broken signatures and hash function in fuzzing so the fuzzer should be able to do this, even if its a bit of work (usually just all-0s and a single byte).

Great, I saw the code in rust-secp256k1 and noticed that with the secp256k1_fuzz flag, there is a fuzzed implementation of secp256k1_ecdsa_verify (https://github.com/rust-bitcoin/rust-secp256k1/blob/master/secp256k1-sys/src/lib.rs#L1828-L1845). So, I think it's fine to remove the manual message building. Working on it now

fuzz: add fuzz target for P2PGossipSync gossip message handling

d29150d

Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>

NishantBansal2003 force-pushed the fuzz-gossip branch from cb9e2a8 to d29150d Compare April 1, 2026 15:52

ldk-reviews-bot requested a review from joostjager April 1, 2026 15:54

ldk-claude-review-bot reviewed Apr 1, 2026

View reviewed changes

TheBlueMatt reviewed Apr 2, 2026

View reviewed changes

TheBlueMatt reviewed Apr 6, 2026

View reviewed changes

fixup! fuzz: add fuzz target for P2PGossipSync gossip message handling

8ea1d00

NishantBansal2003 requested a review from TheBlueMatt April 6, 2026 18:45

		@@ -0,0 +1,462 @@
		//! Test that no series of gossip messages received from peers can result in a crash. We do this

Conversation

NishantBansal2003 commented Apr 1, 2026

Uh oh!

ldk-reviews-bot commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-claude-review-bot Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

ldk-claude-review-bot Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

ldk-claude-review-bot commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-claude-review-bot Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

NishantBansal2003 commented Apr 2, 2026

Uh oh!

ldk-reviews-bot commented Apr 4, 2026

Uh oh!

ldk-reviews-bot commented Apr 6, 2026

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

NishantBansal2003 Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

NishantBansal2003 Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ldk-reviews-bot commented Apr 1, 2026 •

edited

Loading

ldk-claude-review-bot commented Apr 1, 2026 •

edited

Loading

codecov bot commented Apr 1, 2026 •

edited

Loading